Add connections to notebook spawner page #3311

emilys314 · 2024-10-08T12:49:12Z

Closes https://issues.redhat.com/browse/RHOAIENG-13141

https://www.figma.com/design/97akqulXiGIGtvxkdPLjMm/Data-connections-enhancement?node-id=1135-199631&node-type=canvas&t=L8KlrRwsSgc6f4pF-0

Description

Replaces data connections in the notebook spawner page.

If connection types is enable with the feature flag, the spawner page will show the connections form section instead of the old data connections
2 main actions:
- Attach existing connection
- Create new connection
table of attached connections
- backwards compatible with data connections, they should show up as the s3 type.
- shows warning alert if connections with the same env vars exist

Screenshots:

Before

Table form section

Empty state:

With 2 connections:

Conflicts env vars:

notebook UPDATED_NAME being propagated in from the postgres connection type

Modals

How Has This Been Tested?

Set disableConnectionTypes dev flag to false
Have at least an s3 connection type setup from the admin page (or wait until we have OOTB types)
Go to a DS project and go to the workbenches

"Create workbench" to create a new notebook
or "Edit" and existing one to see that version

Open a notebook console and you can run env to look for the env variables from multiple connections

A couple flows for testing (but there are a lot more):

Create connections ahead of time on the project's "Connections" details tab (or data connections) so they show up in the "attach existing connections"
Create a new connection from the notebook spawner "Create connection" button"
Edit an existing notebook with an s3 connection to see those in the table
In the notebook spawner, with an existing connection, click the kebab to detach
In the notebook spawner, with an existing connection, click the kebab to edit and save

Test Impact

Jest tests added for the connections form sub section.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

openshift-ci · 2024-10-08T12:49:17Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-10-08T14:42:10Z

Codecov Report

Attention: Patch coverage is 80.93220% with 45 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (6ec1b1b) to head (d5e9295).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...ens/spawner/connections/ConnectionsFormSection.tsx	81.33%	14 Missing ⚠️
...eens/spawner/connections/DetachConnectionModal.tsx	7.14%	13 Missing ⚠️
...ages/projects/screens/spawner/connections/utils.ts	47.82%	12 Missing ⚠️
...ens/spawner/connections/SelectConnectionsModal.tsx	92.10%	3 Missing ⚠️
.../src/pages/projects/notebook/useNotebooksStates.ts	85.71%	1 Missing ⚠️
...c/pages/projects/screens/spawner/SpawnerFooter.tsx	88.88%	1 Missing ⚠️
...src/pages/projects/screens/spawner/SpawnerPage.tsx	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3311      +/-   ##
==========================================
+ Coverage   84.79%   84.81%   +0.02%     
==========================================
  Files        1315     1326      +11     
  Lines       29491    29650     +159     
  Branches     8056     8100      +44     
==========================================
+ Hits        25006    25147     +141     
- Misses       4485     4503      +18

Files with missing lines	Coverage Δ
frontend/src/components/MultiSelection.tsx	`77.14% <100.00%> (ø)`
.../src/components/table/TableRowTitleDescription.tsx	`100.00% <100.00%> (ø)`
frontend/src/concepts/connectionTypes/utils.ts	`93.27% <100.00%> (+0.55%)`	⬆️
...ts/screens/detail/connections/ConnectionsTable.tsx	`100.00% <100.00%> (ø)`
...screens/detail/connections/ConnectionsTableRow.tsx	`96.15% <100.00%> (ø)`
...ns/spawner/connections/DuplicateEnvVarsWarning.tsx	`100.00% <100.00%> (ø)`
...ontend/src/pages/projects/screens/spawner/const.ts	`100.00% <ø> (ø)`
...r/environmentVariables/useNotebookEnvVariables.tsx	`84.84% <100.00%> (-0.87%)`	⬇️
...ontend/src/pages/projects/screens/spawner/types.ts	`100.00% <100.00%> (ø)`
.../src/pages/projects/notebook/useNotebooksStates.ts	`90.00% <85.71%> (-10.00%)`	⬇️
... and 6 more

... and 51 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ec1b1b...d5e9295. Read the comment docs.

emilys314 · 2024-10-09T14:20:20Z

@simrandhaliw

frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx

frontend/src/pages/projects/screens/spawner/connections/SelectConnectionsModal.tsx

christianvogt · 2024-10-11T21:36:39Z

frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx

+}) => {
+  const [connectionTypes] = useWatchConnectionTypes();
+
+  const [initialNumberConnections] = React.useState(selectedConnections.length);


This should be stored in a ref

Suggested change

const [initialNumberConnections] = React.useState(selectedConnections.length);

const initialNumberConnectionsRef = React.useRef(selectedConnections.length);

christianvogt · 2024-10-15T17:38:39Z

frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx

+      a.metadata.annotations['opendatahub.io/connection-type'].localeCompare(
+        b.metadata.annotations['opendatahub.io/connection-type'],


Display names can differ from their k8s resource name.

Oh good catch. Although I think this is happening in the project details connection tab as well. Should i fix that in this PR or a separate one?

christianvogt · 2024-10-15T17:41:19Z

frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx

+          <FlexItem>
+            <Button
+              data-testid="attach-existing-connection-button"
+              aria-describedby="no-connections-tooltip"


Suggested change

aria-describedby="no-connections-tooltip"

aria-describedby={unselectedConnections.length === ? 'no-connections-tooltip' : undefined}

christianvogt · 2024-10-16T14:18:33Z

/lgtm
/approve

openshift-ci · 2024-10-16T14:18:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Oct 8, 2024

emilys314 force-pushed the add-connections-to-workbench branch from 250146d to 9a17dcd Compare October 8, 2024 13:57

emilys314 force-pushed the add-connections-to-workbench branch 3 times, most recently from 83c4a81 to fa34d84 Compare October 8, 2024 20:49

emilys314 changed the title ~~notebook connection~~ Add connections to notebook spawner page Oct 8, 2024

emilys314 force-pushed the add-connections-to-workbench branch 2 times, most recently from a3bb625 to cc49fd6 Compare October 9, 2024 14:18

emilys314 marked this pull request as ready for review October 9, 2024 14:19

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 9, 2024

openshift-ci bot requested review from DaoDaoNoCode and dpanshug October 9, 2024 14:19

notebook connections

4ad8a3d

emilys314 force-pushed the add-connections-to-workbench branch 4 times, most recently from 0069d14 to 9bf71af Compare October 14, 2024 14:04

Add cypress test

9bf71af

jeff-phillips-18 reviewed Oct 14, 2024

View reviewed changes

frontend/src/pages/projects/screens/spawner/connections/ConnectionsFormSection.tsx Outdated Show resolved Hide resolved

jeff-phillips-18 reviewed Oct 14, 2024

View reviewed changes

small updates

abd3dc8

jeff-phillips-18 approved these changes Oct 15, 2024

View reviewed changes

openshift-ci bot assigned jeff-phillips-18 Oct 15, 2024

openshift-ci bot added the lgtm label Oct 15, 2024

christianvogt reviewed Oct 15, 2024

View reviewed changes

updates part 2

d5e9295

openshift-ci bot removed the lgtm label Oct 16, 2024

openshift-ci bot assigned christianvogt Oct 16, 2024

openshift-ci bot added the lgtm label Oct 16, 2024

openshift-ci bot added the approved label Oct 16, 2024

openshift-merge-bot bot merged commit cdc5b4c into opendatahub-io:main Oct 16, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connections to notebook spawner page #3311

Add connections to notebook spawner page #3311

emilys314 commented Oct 8, 2024 •

edited

Loading

openshift-ci bot commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

emilys314 commented Oct 9, 2024

christianvogt Oct 11, 2024

christianvogt Oct 15, 2024 •

edited

Loading

emilys314 Oct 15, 2024

christianvogt Oct 15, 2024

christianvogt commented Oct 16, 2024

openshift-ci bot commented Oct 16, 2024

	const [initialNumberConnections] = React.useState(selectedConnections.length);
	const initialNumberConnectionsRef = React.useRef(selectedConnections.length);

		a.metadata.annotations['opendatahub.io/connection-type'].localeCompare(
		b.metadata.annotations['opendatahub.io/connection-type'],

	aria-describedby="no-connections-tooltip"
	aria-describedby={unselectedConnections.length === ? 'no-connections-tooltip' : undefined}

Add connections to notebook spawner page #3311

Add connections to notebook spawner page #3311

Conversation

emilys314 commented Oct 8, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

emilys314 commented Oct 9, 2024

christianvogt Oct 11, 2024

Choose a reason for hiding this comment

christianvogt Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

emilys314 Oct 15, 2024

Choose a reason for hiding this comment

christianvogt Oct 15, 2024

Choose a reason for hiding this comment

christianvogt commented Oct 16, 2024

openshift-ci bot commented Oct 16, 2024

emilys314 commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading

christianvogt Oct 15, 2024 •

edited

Loading